Skip to content

[Dynamic dashboard] Improve "has orders" logic #11469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 9, 2024
Merged

[Dynamic dashboard] Improve "has orders" logic #11469

merged 3 commits into from
May 9, 2024

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented May 8, 2024 •
edited
Loading

Description

This PR updates the logic of deciding on the availability of the stats cards, for more information check p1715164179010139-slack-C03L1NF1EA3

Testing instructions

  1. Using a store that has orders.
  2. Clear the app's data (the important tables to clear are OrderEntity and WCOrderStatusOption if you want to do it manually)
  3. Login.
  4. Confirm the stats card are shown
  5. Close and reopen the app
  6. Confirm the stats cards are shown directly without any flicker.

Images/gif

Before After
Screen_recording_20240508_104705.webm
Screen_recording_20240508_112721.webm
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

@hichamboushaba hichamboushaba added type: enhancement A request for an enhancement. feature: dashboard Related to home screen project labels May 8, 2024
@dangermattic
Copy link
Collaborator

dangermattic commented May 8, 2024 •
edited
Loading

1 Warning
⚠️ This PR is assigned to the milestone 18.6. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

hichamboushaba
hichamboushaba commented May 8, 2024
View reviewed changes
WooCommerce/src/main/kotlin/com/woocommerce/android/SiteObserver.kt
Comment on lines +39 to +40
coroutineScope {
launch { fetchPlugins(site) }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to use coroutineScope with launch builders is to allow parallel requests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever!

hichamboushaba
hichamboushaba commented May 8, 2024
View reviewed changes
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/main/MainPresenter.kt
Comment on lines -85 to -89
// Fetch a fresh list of order status options
dispatcher.dispatch(
WCOrderActionBuilder
.newFetchOrderStatusOptionsAction(FetchOrderStatusOptionsPayload(site))
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved to the SiteObserver class to ensure it's called both when the app is launched, and when the selected site is changed.

hichamboushaba
hichamboushaba commented May 8, 2024
View reviewed changes
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/onboarding/StoreOnboardingRepository.kt
Comment on lines +65 to +70
// Update onboarding completed status based on the tasks completion status
if (mobileSupportedTasks.all { it.isComplete }) {
appPrefs.updateOnboardingCompletedStatus(selectedSite.getSelectedSiteId(), true)
} else if (appPrefs.isOnboardingCompleted(selectedSite.getSelectedSiteId())) {
appPrefs.updateOnboardingCompletedStatus(selectedSite.getSelectedSiteId(), false)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this logic here instead of ShouldShowOnboarding was to make sure we don't miss updating the status when the onboarding tasks list is empty, this happened to me on one of my sites, and the onboarding card kept being shown then dismissed on each launch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I saw this as well on my site and planned to look into it.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 8, 2024 •
edited
Loading

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
FlavorJalapeno
Build TypeDebug
Commitb4acb19
Direct Downloadwoocommerce-prototype-build-pr11469-b4acb19.apk

@codecov-commenter
Copy link

codecov-commenter commented May 8, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 45 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (trunk@9bbf58a). Click here to learn what that means.
Report is 7 commits behind head on trunk.

Files Patch % Lines
...ain/kotlin/com/woocommerce/android/SiteObserver.kt 0.00% 23 Missing ⚠️
...oid/ui/dashboard/data/ObserveStatsWidgetsStatus.kt 0.00% 16 Missing ⚠️
...android/ui/onboarding/StoreOnboardingRepository.kt 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             trunk   #11469   +/-   ##
========================================
  Coverage         ?   40.82%           
  Complexity       ?     5219           
========================================
  Files            ?     1072           
  Lines            ?    62936           
  Branches         ?     8615           
========================================
  Hits             ?    25692           
  Misses           ?    34927           
  Partials         ?     2317

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hichamboushaba hichamboushaba marked this pull request as ready for review May 8, 2024 11:58
@hichamboushaba hichamboushaba requested a review from 0nko May 8, 2024 11:58
@hichamboushaba
Use the order status options to check if store has orders
6f815b4 
This is an improvement when the app doesn't any cached orders to tell if the store has orders or not, and would avoid flickering on the main screen during initial load.
@hichamboushaba
Move logic of marking onboarding as complete to the repository
9c6bb38 
This fixes issues where we miss updating the status when we don't have any tasks.
@hichamboushaba
Improve logic of fetching status options
b4acb19 
The new logic means we'll fetch on each app launch in addition to site switching.
@hichamboushaba hichamboushaba force-pushed the dynamic-dashboard/improve-has-orders-logic branch from 8e5b448 to b4acb19 Compare May 8, 2024 12:00
@hichamboushaba hichamboushaba added this to the 18.6 milestone May 8, 2024
@0nko 0nko self-assigned this May 8, 2024
0nko
0nko approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@0nko 0nko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm 👏

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/onboarding/StoreOnboardingRepository.kt
Comment on lines +65 to +70
// Update onboarding completed status based on the tasks completion status
if (mobileSupportedTasks.all { it.isComplete }) {
appPrefs.updateOnboardingCompletedStatus(selectedSite.getSelectedSiteId(), true)
} else if (appPrefs.isOnboardingCompleted(selectedSite.getSelectedSiteId())) {
appPrefs.updateOnboardingCompletedStatus(selectedSite.getSelectedSiteId(), false)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I saw this as well on my site and planned to look into it.

@0nko 0nko merged commit bd930eb into trunk May 9, 2024
3 of 4 checks passed
@0nko 0nko deleted the dynamic-dashboard/improve-has-orders-logic branch May 9, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0nko 0nko 0nko approved these changes

Assignees

@0nko 0nko

Labels
feature: dashboard Related to home screen project type: enhancement A request for an enhancement.
Projects
None yet
Milestone
18.6
Development

Successfully merging this pull request may close these issues.
5 participants
@hichamboushaba @dangermattic @wpmobilebot @codecov-commenter @0nko